New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add "Render all markdown cells" command #6029
Add "Render all markdown cells" command #6029
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rahulpshah! A few comments, but overall this looks good.
packages/mainmenu/src/run.ts
Outdated
@@ -74,6 +74,8 @@ export namespace IRunMenu { | |||
*/ | |||
runAll?: (widget: T) => Promise<void>; | |||
|
|||
runAllMarkdown?: (widget: T) => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ICodeRunner
interface is meant to expose a way for many different types of things that run code to hook into the "Run" menu. In this case, I think this command is pretty specific to notebooks, and not a generic "code-running" activity. So I'd recommend adding the command directly to the menu, rather than through the codeRunner
.
@@ -119,6 +119,8 @@ namespace CommandIDs { | |||
|
|||
export const runAllBelow = 'notebook:run-all-below'; | |||
|
|||
export const runAllMarkdown = 'notebook:run-all-markdown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to name it render-all-markdown
/renderAllMarkdown
for consistency with the label?
packages/notebook/src/actions.tsx
Outdated
notebook.activeCellIndex = index; | ||
} | ||
}); | ||
if (notebook.widgets[notebook.activeCellIndex].model.type !== 'markdown') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make this a bit simpler by accessing notebook.activeCell.model.type
@@ -2062,6 +2076,12 @@ function populateMenus( | |||
() => void 0 | |||
); | |||
}, | |||
runAllMarkdown: current => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where you would add a group containing the renderAllMarkdown
to the "Run" menu, rather than adding it to the codeRunners
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow review turnaround @rahulpshah, this is looking good. A few more comments oriented around cleanup, then I think we should be good to go.
@@ -92,6 +92,8 @@ export namespace CommandIDs { | |||
|
|||
export const runAll = 'runmenu:run-all'; | |||
|
|||
export const runAllMarkdown = 'runmenu:run-all-markdown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this ID should be here at all -- there is no 'runmenu:run-all-markdown'
command defined as far as I see.
const runAllGroup = [ | ||
CommandIDs.runAll, | ||
CommandIDs.restartAndRunAll, | ||
CommandIDs.runAllMarkdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed.
// Add a renderAllMarkdown group to the run menu. | ||
const renderAllMarkdown = [ | ||
CommandIDs.renderAllMarkdown, | ||
CommandIDs.run, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove run
and runInConsole
here? The first is redundant with existing menu items, and the second should probably have its own discussion. I think it's fine for renderAllMarkdown
to have its own menu group.
Hi @ian-r-rose, I have made the changes you requested but I am unsure why the windows build is failing. Any pointers? |
@rahulpshah That's probably due to the flakiness of our CI. I can try to restart it to see if it passes. |
Looks great, thanks @rahulpshah! |
Fixes #6017 - Adds an option to render markdown cells.
On clicking the button, It renders all the markdown cells in the notebook. The PR makes sure that the active cell is not executed if it is not markdown.
Run Menu:
Sample Notebook
Before:
After:
@ian-r-rose Let me know your thoughts on this PR? Is there something that needs to be added?